Skip to content

[History server] Update history server examples and defaults - #5080

Open
chiayi wants to merge 13 commits into
ray-project:masterfrom
chiayi:history-server
Open

[History server] Update history server examples and defaults#5080
chiayi wants to merge 13 commits into
ray-project:masterfrom
chiayi:history-server

Conversation

@chiayi

@chiayi chiayi commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Why are these changes needed?

This PR updates the event port default to 8084 from 8080 which will clash with dashboard port. Along with changing the default, this PR also update examples to use the env var instead of the flags. And since the entrypoint for the collector and historyserver is not part of the image, we no longer need the command.

It was also found that RAY_ROOT_DIR was not added to historyserver, this PR adds it.

Edit: Update scope

Related issue number

Part of #4187

Labels

  • If this PR has user-facing changes that require documentation updates at release time, I have added the doc-updates-required label.
  • If this PR contains breaking changes, I have added the breaking-change label.

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • This PR is not tested :(

Manual test instructions

Ran the yaml against a live kubernetes cluster.

@chiayi
chiayi marked this pull request as ready for review August 5, 2026 00:34
@chiayi

chiayi commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

The one line change in the ray controller unit test was to fix:
image

- name: RAY_DASHBOARD_AGGREGATOR_AGENT_EVENTS_EXPORT_ADDR
value: "http://localhost:8084/v1/events"
- name: RAY_DASHBOARD_AGGREGATOR_AGENT_EXPOSABLE_EVENT_TYPES
- name: RAY_DASHBOARD_AGGREGATOR_AGENT_PUBLISHER_HTTP_ENDPOINT_EXPOSABLE_EVENT_TYPES

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this change?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC this is just a variable update starting from 2.55+ to tell which event to export.

@machichima machichima Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the change was reverted? ray-project/ray#59965
We should keep RAY_DASHBOARD_AGGREGATOR_AGENT_EXPOSABLE_EVENT_TYPES here

TASK_PROFILE_EVENT,DRIVER_JOB_DEFINITION_EVENT,DRIVER_JOB_LIFECYCLE_EVENT,
ACTOR_DEFINITION_EVENT,ACTOR_LIFECYCLE_EVENT,NODE_DEFINITION_EVENT,NODE_LIFECYCLE_EVENT"
image: rayproject/ray:2.52.0
image: rayproject/ray:2.55.0

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use 2.56 while we're at it?

Comment thread historyserver/config/raycluster-azureblob.yaml Outdated

@win5923 win5923 Aug 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering should we still keep this sample yaml or just change to use spec.historyServerOptions.collectorOptions instead, so the user don't need to manually set the envs and args.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This requires RayClusterHistoryServer feature gate. I think we could have a separate sample for this?

if features.Enabled(features.RayClusterHistoryServer) && instance.Spec.HistoryServerOptions != nil && instance.Spec.HistoryServerOptions.CollectorOptions != nil {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread historyserver/config/raycluster-gcs.yaml
- name: collector
image: collector:v0.1.0
image: quay.io/kuberay/collector:nightly
imagePullPolicy: IfNotPresent

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use nightly, I think we need to set imagePullPolicy: Always, otherwise it will never pull the newest image. Also for other files.
Actually I think we can come back and update this after release to pin the specific version

Comment thread historyserver/config/historyserver.yaml Outdated
Comment on lines 55 to 58

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we also remove command here?

@chiayi chiayi Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh you know what, good point. So the env variables have not been converted on the history server side. History server will need to check for the RAY_ROOT_DIR env var. And I think it's small enough to add the change here (this PR) as well.

FYI: @andrewsykim

@machichima

Copy link
Copy Markdown
Collaborator

We'll need to update the ray docs after this is merge. Can do it together with #5085

@machichima

Copy link
Copy Markdown
Collaborator

CI failed with unable to start container process: error during container init: exec: \"--ray-cluster-namespace=test-ns-c5rxh\": executable file not found in $PATH"}. I think it's related to removing command from the YAML.

@Future-Outlier Future-Outlier left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @chiayi
plz merge this PR #5082 before you get this PR merged, tks

@Future-Outlier Future-Outlier left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am thining that we should combine historyserver/config/raycluster.yaml and historyserver/config/rayjob.yaml to RayJob example, WDYT?

and also these 2 files might be better if we turn then to rayjob example too.
historyserver/config/raycluster-azureblob.yaml
historyserver/config/raycluster-gcs.yaml

@andrewsykim

Copy link
Copy Markdown
Member

+1 to use ephemeral RayJob with spec.shutdownAfterJobFinishes or deletion policy API where it makes sense

@chiayi

chiayi commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Hi, @chiayi plz merge this PR #5082 before you get this PR merged, tks

I think we can maybe combine that PR with this one? I can change it to all here and this PR already have the version update.

@chiayi

chiayi commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

and also these 2 files might be better if we turn then to rayjob example too.
historyserver/config/raycluster-azureblob.yaml
historyserver/config/raycluster-gcs.yaml

Will convert these to rayjobs

@chiayi

chiayi commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

So historyserver/config/raycluster-azureblob.yaml is hard coded into the e2e tests and I think it will require more additional code changes. I think I will leave it as is as I am not familiar with azure configurations,

Comment thread historyserver/config/rayjob-gcs.yaml
Comment thread historyserver/config/raycluster.yaml
Comment thread historyserver/config/raycluster.yaml Outdated
@chiayi

chiayi commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Some of these change leading to more changes and bloating this PR. I will not be changing the image and the azureblob yaml to avoid adding to this PR.

FYI: @machichima @andrewsykim

containers:
- env:
- name: RAY_TMP_ROOT
value: &rayTmpRoot /tmp/ray

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should add a comment at the top of this file that it must be run with envsubst with an example

Comment thread historyserver/config/rayjob-gcs.yaml Outdated
- name: STORAGE_BACKEND
value: "gcs"
- name: RAY_ROOT_DIR
value: "log"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one seems a bit odd, why do we use "log" for RAY_ROOT_DIR

@chiayi chiayi Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly do not remember the reason for setting root dir to logs. But we can probably just change it to root

@andrewsykim andrewsykim Aug 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think root is a valid value for RAY_ROOT_DIR either, what's the default value if we don't set it? If that works just don't set it here?

Comment thread historyserver/config/raycluster.yaml Outdated
Comment thread historyserver/config/rayjob-gcs.yaml Outdated
- name: STORAGE_BACKEND
value: "gcs"
- name: RAY_ROOT_DIR
value: "root"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think "root" will work either? Is RAY_ROOT_DIR referring to Ray's root directory or the root directory used in storage? If it's storage, this variable name is confusing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's referring to the root directory in storage. So maybe STORAGE_ROOT_DIR instead?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I would rename to STORAGE_ROOT_DIR. Also when it's empty it should just use the specified bucket's root dir

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chiayi please fix this in a fast follow-up

fieldPath: status.podIP
- name: FQ_RAY_IP
value: $(RAY_CLUSTER_NAME)-head-svc.$(RAY_CLUSTER_NAMESPACE).svc.cluster.local
- name: RAY_TMP_ROOT

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to set this? I think this will almost always be /tmp/ray

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have it as an option because it's also an option for rayclusters. In case users decide to change the raycluster temp-dir.

Comment thread historyserver/config/rayjob-gcs.yaml
Comment thread historyserver/config/rayjob-gcs.yaml Outdated
Comment thread historyserver/config/rayjob-gcs.yaml Outdated
- name: code-sample
mountPath: /home/ray/samples
- name: collector
image: collector:v0.1.0

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use the official image?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so that the ci will use the PR's version of history server to run the tests. As @machichima mentioned here: #5080 (comment) we should separate it out so that there is a specific dir for test suite, but I think that should be a separate PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooops. correction, this is rayjob and not raycluster. And raycluster-gcs isn't currently used in e2e either.

mountPath: *rayTmpRoot
- name: collector
image: ${COLLECTOR_IMAGE}
image: collector:v0.1.0

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would need to unify here. Now we have two versions, one setting collector:v0.1.0 and the other set the nightly image (in historyserver/config/rayjob-gcs.yaml).
Are we going to:

  1. keep collector:v0.1.0 here and update after release for the release version
  2. use nightly version with image pull policy always
  3. Have an env var for user to inject the image

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about leaving it as is so ${COLLECTOR_IMAGE} and update once we release? so a combination of 3 and 1. So we don't change anything for the image here until release.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, let's do this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! will do this!

Comment thread historyserver/config/rayjob-gcs.yaml Outdated
Comment thread historyserver/config/historyserver.yaml
Comment thread historyserver/config/rayjob-gcs.yaml Outdated
Comment on lines +25 to +26
imagePullSecrets:
affinity:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we leave empty fields here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, so the raycluster part of the spec is actually copied over from raycluster.yaml. I guess they are currently just empty placeholders.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, should we just remove them?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do!

- name: STORAGE_BACKEND
value: "gcs"
- name: RAY_ROOT_DIR
value: "storage-root"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's mismatch in ray root dir. Here we use storage-root but in other places (e.g. historyserver/config/raycluster-azureblob.yaml) we use log

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The log is actually hard coded in the e2e tests for azure and s3 and did not want to add to this PR even more. And I updated the raycluster-gcs because it is used in the ray doc/guide and did not want to cause confusion when users view them.

What we could do is when we open a RAY_ROOT_DIR fast followup change to STORAGE_ROOT_DIR, we can also make the changes to the e2e test as well? So no change here for the time being to avoid confusion.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SG

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

Reviewed by Cursor Bugbot for commit aa941ee. Configure here.

TASK_PROFILE_EVENT,DRIVER_JOB_DEFINITION_EVENT,DRIVER_JOB_LIFECYCLE_EVENT,
ACTOR_DEFINITION_EVENT,ACTOR_LIFECYCLE_EVENT,NODE_DEFINITION_EVENT,NODE_LIFECYCLE_EVENT"
image: rayproject/ray:2.52.0
value: "ALL"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incomplete Ray 2.56 event export config

Medium Severity

After upgrading these samples to Ray 2.56.0 and switching exposable event types to ALL, only RAY_DASHBOARD_AGGREGATOR_AGENT_EXPOSABLE_EVENT_TYPES is set. The GCS samples updated in the same change also set RAY_DASHBOARD_AGGREGATOR_AGENT_PUBLISHER_HTTP_ENDPOINT_EXPOSABLE_EVENT_TYPES, and the removed comments previously called out that newer Ray needs that publisher variable for HTTP export to the collector.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit aa941ee. Configure here.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it should be RAY_DASHBOARD_AGGREGATOR_AGENT_EXPOSABLE_EVENT_TYPES instead of RAY_DASHBOARD_AGGREGATOR_AGENT_PUBLISHER_HTTP_ENDPOINT_EXPOSABLE_EVENT_TYPES @chiayi

Comment thread historyserver/config/rayjob-gcs.yaml Outdated
- name: historyserver
mountPath: *rayTmpRoot
- name: collector
image: quay.io/kuberay/collector:nightly

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to #5080 (comment)
I think we also need to update here?

Comment thread historyserver/config/rayjob-gcs.yaml Outdated
- name: code-sample
mountPath: /home/ray/samples
- name: collector
image: quay.io/kuberay/collector:nightly

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Comment thread historyserver/config/rayjob-gcs.yaml Outdated
imagePullPolicy: IfNotPresent
securityContext:
allowPrivilegeEscalation: true
privileged: true

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to use priviledged containers? I really hope this is no longer required for Beta

Comment thread historyserver/config/rayjob-gcs.yaml Outdated
- name: historyserver
mountPath: *rayTmpRoot
tolerations:
- key: ray

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a weird toleration to have here, do we need it?

@chiayi

chiayi commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Followups:

  1. Fast followup to update the RAY_ROOT_DIR to something like STORAGE_ROOT_DIR to avoid configuration confusion. Also set the value to storage-root from logs. This will require changing the hardcode in some of the tests.
  2. Separate the samples directory to into samples/ and testdata/.
    - So we no longer hardcode the images
    - Sample no longer need to be v2.55
    - Overall so that updating the samples won't break CI.

- name: STORAGE_BACKEND
value: "gcs"
- name: RAY_ROOT_DIR
value: "storage-root"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now can we just not set RAY_ROOT_DIR? In this case the default behavior should be using the root of the provided bucket. Or whichever subdirectory the user specifies in GCS_BUCKET gs://my-bucket/historyserver/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants